Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for comments #2

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Request for comments #2

wants to merge 18 commits into from

Conversation

gipi
Copy link
Contributor

@gipi gipi commented Jan 4, 2013

In this branch I tried to implement some ideas

Media files

Previously the javascript files was included directly in the easy_maps/map.html template but when more than one {% easy_map %} was used, multiple instances of Google maps javascript could create problems (chrome console complains about this).

In @1f0a767 I used the Media class for the widget used in the rendering of the address field.

Settings

Added (@8f71c84 and @bf15559 to be squashed together) a settings.py containing the default to be used when a new instance is created (also used to center the map). Together with the changes in @39645fd is possible to avoid the saving of an instance with an empty address.

Interactivity

I don't like that for see where the marker will be placed I need to save the instance and come back, so I added some javascript functionality: first of all the update button that queries the geolocalization URL (a custom view using JSON that responds with the data used to full the form field)

em1

This is introduced in @5ca4a70 where a geo/ URL is added to the Admin url patterns.

Also I would like to have the coordinates updated when I move the marker on the map (see @d1a0d5a).

Inlines

An admin class to be used with inlines is added with the same functionalities descripted above, this has required some modification on how functions and HTML element classes are named.

There is some example models added in @d1a0d5a that use it.

Multiple markers

The easy_maps tag is improved so to accept also a list of Address instances that will be placed in the map (see @39645fd).

Change list

Since the primary data of this application is geographical data I would like to have a change list page in the admin that contains a map at a zoom level such that all the point are immediately visualized. This (@1755f7d) is a very dirty hack and in future I'd love to implement some interaction between the normal Django table entries and the markers.

Conclusion

This is not code I'm proud of, it's a little messy and should be reorganized but to avoid useless work I need to know what do you think about these ideas.

@kmike
Copy link
Collaborator

kmike commented Jan 5, 2013

Hi Gianluca,

Thanks for working on this! I'll comment on features now.

Media files

Currently, it is possible to use a custom template with nulled api_js block, e.g.:

# my_map.html
{% extends "easy_maps/map.html" %}
{% block api_js %}{% endblock %}

# user template
<script type="text/javascript" src="https://maps.google.com/maps/api/js?sensor=false"></script>

{% easy_map ... using "my_map.html" %}
{% easy_map ... using "my_map.html" %}

This doesn't require using custom widget for a form field, and this also doesn't require using a form.
1f0a767 brokes backwards-compatibility by requiring user to include <script> tag manually or using a form with AddressWithMapWidget.

I don't want django-easy-maps to be tied to forms, and I really like that it is possible to just drop {% easy_map "<My address>" 300 300 %} and get a map (e.g. at a "Contacts" page).

Currently, the inlined javascript may become an issue if multiple instances of the same map are displayed (because the template is parametrized by map.pk), but this is rarely an issue (and this limitation still apply in your branch).

But using Media in admin widget is a good idea. Maybe use an another template while rendering widgets, the one which assumes the external javascript is loaded? It is also better to put easy_maps javascript files to static/easy_maps/js or static/easy_maps, not to static/js because the latter may clash with user files.

Settings

+1 for defaults in settings, this may be useful.

Interactivity

This is a really useful feature, but I think it is better to move work to the client side. Shouldn't we use javascript geocoder instead of geopy/json api in this case ( see https://developers.google.com/maps/documentation/geocoding/#ReverseGeocoding )? I'd prefer the interactivity to be implemented purely in javascript.

Inlines

+1

Multiple markers

I like the idea. Currently easy_maps tag accepts either string or Address instance; is it possible to have multiple markers with just strings or form fields? What should syntax look like?

Change list

I like the idea.


By the way, https://github.com/gipi/django-easy-maps/blob/list-addresses/easy_maps_tests/test_app/models.py is not how django-easy-maps was intended to be used :) The idea was to write

class Shop(models.Model):
    address = models.CharField(max_length=255)
    brand = models.ForeignKey(Brand)

(that's what existing sites probably have) and then use {% easy_map shop.address 300 300 %} tag to render the map for shop's address; Address model is more like a geocoder cache with some fine-tuning options.

@gipi
Copy link
Contributor Author

gipi commented Jan 8, 2013

Thanks for your reply.

Address model is more like a geocoder cache with some fine-tuning options.

this explains a lot to me and I can say that my intention was to improve the admin-side of this application :)

Media Files

IHMO is better to avoid to include the Google's script in the widget and to tell the user to include it in the main template, otherwise we could add an optional version of the tag like

{% easy_map address 300 300 with_js=False %}

(using a custom template for a common case is annoying).

1f0a767 brokes backwards-compatibility by requiring user to include script tag manually or using a form with AddressWithMapWidget.

could be milestoned for a next major version a change in that, maybe indicating it in the documentation, obviously if you are ok with this.

Currently, the inlined javascript may become an issue if multiple instances of the same map are displayed (because the template is parametrized by map.pk), but this is rarely an issue (and this limitation still apply in your branch).

I don't understand what you mean, could you make an example?

It is also better to put easy_maps javascript files to static/easy_maps/js or static/easy_maps, not to static/js because the latter may clash with user files.

ok, I'll commit this

Interactivity

If we move the geocoding code client side (hence removing geo.py) can be problematic in browsers without javascript enabled.

Multiple markers

I like the idea. Currently easy_maps tag accepts either string or Address instance; is it possible to have multiple markers with just strings or form fields? What should syntax look like?

{% easy_map "address1" "address2" 300 300 %}

I don't understand the form fields stuffs, could you elaborate that?

BTW since there are some things you are ok with, you could indicate me what commits keep (or modify) so that I can reorganize this branch with only them in it in order to allow you to merge and maybe you could open an issue for each argument in this discussion with the guideline of implementations.

@kmike
Copy link
Collaborator

kmike commented Jan 20, 2013

Hi @gipi and sorry for a long delay!

Media files

I like the idea of "with_js" attribute. However there are 3 possibilities:

a) include all the js (script tag with a link to Google and our js map initialization code);
b) include only our map init code;
c) include nothing.

"with_js" should do (b) - but then it needs a better name because some js would still be included :) Any ideas?

I think we shouldn't change the default (with_js=True) though.

Currently, the inlined javascript may become an issue if multiple instances of the same map are displayed (because the template is parametrized by map.pk), but this is rarely an issue (and this limitation still apply in your branch).
I don't understand what you mean, could you make an example?

I mean passing the same Address instance to several easy_maps tags on the same page. They would have the same pk and js code may become incorrect. I don't know if this is a real-life issue.

Interactivity

Interactivity wouldn't work without js anyway, would it? Client-side requests make more sense because they provide the same capabilities and don't block the server. I think we should do geocoding (user enters the address) both on server side (when interactivity is not available or the address is hard-coded in template) and on client side (when interactivity is enabled), and we should do reverse geocoding (when user clicks the map) only on client side; the UI is a tricky part for geocoding. I think the implementation may include some hidden fields.

Multiple markers

I mean "model fields" :) The syntax for static addresses looks good for me. But what with querysets? For example, we have some Shops in database; each shop has an "address" TextField. How to create a map with their addresses on the same map?


I think that settings for the default map center are ready (btw, I like EASY_MAPS_CENTER = (45.0677201, 7.6825531) more, but feel free to EASY_MAPS_CENTER_LAT if you want); other things need some work. It'll be great if you split the features so that we can discuss them faster.

@gipi
Copy link
Contributor Author

gipi commented Jan 21, 2013

ok, I'll create a branch of each feature; I think for the end of the week maybe :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants